Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added epub support #154

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added epub support #154

wants to merge 7 commits into from

Conversation

cvanelteren
Copy link

@cvanelteren cvanelteren commented Oct 24, 2024

Can upload epubs to remarkable via the api. Fixes #144

@cvanelteren
Copy link
Author

@GjjvdBurg kind nudge to get this moving

Copy link
Owner

@GjjvdBurg GjjvdBurg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @cvanelteren! I've left a few comments with suggestions. The main suggestion is that EPUB support can be handled by the LocalFile provider by modifying the base class, which seems simpler. Let me know what you think and whether you're up for making those changes!

paper2remarkable/providers/__init__.py Outdated Show resolved Hide resolved
paper2remarkable/ui.py Outdated Show resolved Hide resolved
paper2remarkable/providers/epub.py Outdated Show resolved Hide resolved
paper2remarkable/ui.py Outdated Show resolved Hide resolved
paper2remarkable/ui.py Outdated Show resolved Hide resolved
@cvanelteren
Copy link
Author

Will check the PR tomorrow again. I abstracted some code but the build seems to be failing; local epub files are pushed to the remarkable with the integrated code.

@cvanelteren
Copy link
Author

Same tests pass on the main @GjjvdBurg

@GjjvdBurg
Copy link
Owner

Hi @cvanelteren, thanks for making the changes, I think it looks good! The build is failing on formatting errors, do you have a chance to fix those? Also, it would be great if we could have a unit test that verifies epub functionality. I'm wondering if there are any good sources for that, maybe something like Project Gutenberg? That could imply adding a provider for it though, which does increase the scope of this PR so we could also merge this after fixing the formatting errors and then add that provider/test later

@cvanelteren
Copy link
Author

Will look into it no worries.

@cvanelteren cvanelteren requested a review from GjjvdBurg January 5, 2025 12:22
@GjjvdBurg
Copy link
Owner

Thanks for adding the test @cvanelteren! Unfortunately the change to the PdfUrl provider causes the existing test_pdfurl_1 and test_pdfurl_2 tests to fail. It may help to run the unit tests locally to confirm

@cvanelteren
Copy link
Author

cvanelteren commented Jan 6, 2025

@GjjvdBurg did check it beforehand but lazily only checked what I added. There was a typo in the check. There are some other tests that fail relating to filenames that changed but I believe that is outside the scope of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Epub support wanted
2 participants